Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[15.0][FIX] hr_employee_document: Remove record rules from hr.employee to avoid side effects.. #1271

Merged
merged 1 commit into from
Aug 22, 2023
Merged

[15.0][FIX] hr_employee_document: Remove record rules from hr.employee to avoid side effects.. #1271

merged 1 commit into from
Aug 22, 2023

Conversation

victoralmau
Copy link
Member

@victoralmau victoralmau commented Jul 21, 2023 •
edited
Loading

Remove record rules from hr.employee to avoid side effects.

Example of use case:

  • An employee X is created and a Manager Y (parent_id) is defined.
  • User X creates an expense report (hr.expense.sheet).
  • The "Submit to manager" button is clicked.
  • A permissions error is displayed when accessing employee Y (hr.employee) because the record rules that this module had added do not allow access to that record.

Solution:
This change overrides some things to allow the user's employee attachments to be displayed.

Please @pedrobaeza can you review it?

@Tecnativa TT44536

@pedrobaeza
Copy link
Member

You have to put the steps to reproduce the problem in the commit message.

Apart from that, why that ACLs was there? (isn't they record rules, not ACL?) Seeing a bit the code, it seems you are solving it acting at code level. Please explain all of this in the commit message, docstring, etc.

@victoralmau victoralmau force-pushed the 15.0-fix-hr_employee_document-TT44536 branch from 6e7570c to e67de4b Compare July 21, 2023 11:45
@victoralmau victoralmau changed the title [15.0][FIX] hr_employee_document: Remove ACLs from hr.employee to avoid side effects (with hr_expense for example). [15.0][FIX] hr_employee_document: Remove record rules from hr.employee to avoid side effects.. Jul 21, 2023
@victoralmau
Copy link
Member Author

Sorry, changed the commit message and explained a use case.

@pedrobaeza pedrobaeza added this to the 15.0 milestone Aug 8, 2023
pedrobaeza
pedrobaeza requested changes Aug 8, 2023
View reviewed changes
hr_employee_document/models/ir_attachment.py Outdated
_inherit = "ir.attachment"

@api.model
def _search(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security shouldn't be handled in _search method. This is a security hole, as you can browse directly some random records.

@victoralmau victoralmau force-pushed the 15.0-fix-hr_employee_document-TT44536 branch 2 times, most recently from 243dd84 to f37ef07 Compare August 14, 2023 15:15
@victoralmau victoralmau marked this pull request as draft August 14, 2023 15:15
@victoralmau
Copy link
Member Author

I have changed the approach to try to solve the problem in another way, although there is still some detail to be solved, what do you think about this way?

@victoralmau victoralmau force-pushed the 15.0-fix-hr_employee_document-TT44536 branch from f37ef07 to 0a978c0 Compare August 16, 2023 06:11
@victoralmau victoralmau force-pushed the 15.0-fix-hr_employee_document-TT44536 branch from 0a978c0 to bc586f9 Compare August 21, 2023 06:17
@victoralmau victoralmau marked this pull request as ready for review August 21, 2023 06:18
@victoralmau
[FIX] hr_employee_document: Remove record rules from hr.employee to a…
5ec7364 
…void side effects.

The purpose of this module is to allow a basic user (without hr permissions) to view
the attachments related to his employee (hr.employee).

By default a basic user cannot access to hr.employee model and therefore
cannot see the attachments (ir.attachment) linked to it.

This change overrides some things to allow the user's employee attachments to be displayed.

TT44536
@victoralmau victoralmau force-pushed the 15.0-fix-hr_employee_document-TT44536 branch from bc586f9 to 5ec7364 Compare August 22, 2023 10:01
chienandalu
chienandalu approved these changes Aug 22, 2023
View reviewed changes
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza
Copy link
Member

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 15.0-ocabot-merge-pr-1271-by-pedrobaeza-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit f899133 into OCA:15.0 Aug 22, 2023
6 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 2811e28. Thanks a lot for contributing to OCA. ❤️

@victoralmau victoralmau mentioned this pull request Aug 22, 2023
Merged
@kaynnan kaynnan mentioned this pull request Nov 21, 2023
Merged
@marcelsavegnago marcelsavegnago mentioned this pull request Nov 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chienandalu chienandalu chienandalu approved these changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
15.0
Development

Successfully merging this pull request may close these issues.
4 participants
@victoralmau @pedrobaeza @OCA-git-bot @chienandalu